lib/kernel-args: Store kernel args as key/value entries

Define an `OstreeKernelArgsEntry` structure, which holds
both the key and the value. The kargs order array stores
entries for each key/value pair, instead of just the keys.
The hash table is used to locate entries, by storing
entries in a pointer array for each key. The same public
interface is preserved, while maintaining ordering
information of each key/value pair when
appending/replacing/deleting kargs.

Fixes: #1859

diff --git a/src/libostree/ostree-kernel-args.c b/src/libostree/ostree-kernel-args.c
index dbf2ec8a112a3c5d65cebb2c6d0f7804630e47d9..c6300823dc122144bc99f4942afe9699ff880151 100644 (file)
--- a/src/libostree/ostree-kernel-args.c
+++ b/src/libostree/ostree-kernel-args.c
@@ -30,6 +30,111 @@ struct _OstreeKernelArgs {
   GHashTable *table;
 };
 
+struct _OstreeKernelArgsEntry {
+  char *key;
+  char *value;
+};
+
+char *
+_ostree_kernel_args_entry_get_key (const OstreeKernelArgsEntry *e)
+{
+  return e->key;
+}
+
+char *
+_ostree_kernel_args_entry_get_value (const OstreeKernelArgsEntry *e)
+{
+  return e->value;
+}
+
+void
+_ostree_kernel_args_entry_set_key (OstreeKernelArgsEntry *e,
+                                   char  *key)
+{
+  e->key = key;
+}
+
+void
+_ostree_kernel_args_entry_set_value (OstreeKernelArgsEntry *e,
+                                     char  *value)
+{
+  e->value = value;
+}
+
+char *
+_ostree_kernel_args_get_key_index (const OstreeKernelArgs *kargs,
+                                   int i)
+{
+  OstreeKernelArgsEntry *e = kargs->order->pdata[i];
+  return e->key;
+}
+
+char *
+_ostree_kernel_args_get_value_index (const OstreeKernelArgs *kargs,
+                                     int i)
+{
+  OstreeKernelArgsEntry *e = kargs->order->pdata[i];
+  return e->value;
+}
+
+OstreeKernelArgsEntry *
+_ostree_kernel_args_entry_new (void)
+{
+  return g_new0 (OstreeKernelArgsEntry, 1);
+}
+
+void
+_ostree_kernel_args_entry_value_free (OstreeKernelArgsEntry *e)
+{
+  g_clear_pointer (&e->value, g_free);
+}
+
+/* Free the value field, and the entry.  This should be set as the free
+ * function, for all pointer arrays stored in the hash table.
+ */
+static void
+kernel_args_entry_free_from_table (gpointer data)
+{
+  OstreeKernelArgsEntry *e = data;
+  // The hash table owns the key; do not free it here.
+  g_free (_ostree_kernel_args_entry_get_value (e));
+  g_free (e);
+}
+
+static gboolean
+kernel_args_entry_value_equal (gconstpointer data,
+                               gconstpointer value)
+{
+  const OstreeKernelArgsEntry *e = data;
+  return g_strcmp0 (_ostree_kernel_args_entry_get_value (e), value) == 0;
+}
+
+static gboolean
+kernel_args_entry_key_equal (gconstpointer data,
+                             gconstpointer key)
+{
+  const OstreeKernelArgsEntry *e = data;
+  return g_strcmp0 (_ostree_kernel_args_entry_get_key (e), key) == 0;
+}
+
+static void
+kernel_args_entry_replace_value (OstreeKernelArgsEntry *e,
+                                 const char *value)
+{
+  g_assert (e);
+  _ostree_kernel_args_entry_value_free (e);
+  _ostree_kernel_args_entry_set_value (e, g_strdup (value));
+}
+
+static void
+kernel_args_remove_entries_from_order (GPtrArray *order,
+                                       GPtrArray *entries)
+{
+  g_assert (entries);
+  for (int i = 0; i < entries->len; i++)
+    g_assert (g_ptr_array_remove (order, entries->pdata[i]));
+}
+
 static char *
 split_keyeq (char *arg)
 {
@@ -82,9 +187,11 @@ ostree_kernel_args_new (void)
 {
   OstreeKernelArgs *ret;
   ret = g_new0 (OstreeKernelArgs, 1);
-  ret->order = g_ptr_array_new_with_free_func (g_free);
+  /* Hash table owns the kernel args entries, since it uses keys to index,
+   * and its values are used to locate entries in the order array. */
   ret->table = g_hash_table_new_full (g_str_hash, g_str_equal,
-                                      NULL, (GDestroyNotify)g_ptr_array_unref);
+                                      g_free, (GDestroyNotify)g_ptr_array_unref);
+  ret->order = g_ptr_array_new_with_free_func (NULL);
   return ret;
 }
 
@@ -194,10 +301,10 @@ ostree_kernel_args_new_replace (OstreeKernelArgs *kargs,
   const char *key = arg_owned;
   const char *val = split_keyeq (arg_owned);
 
-  GPtrArray *values = g_hash_table_lookup (kargs->table, key);
-  if (!values)
+  GPtrArray *entries = g_hash_table_lookup (kargs->table, key);
+  if (!entries)
     return glnx_throw (error, "No key '%s' found", key);
-  g_assert_cmpuint (values->len, >, 0);
+  g_assert_cmpuint (entries->len, >, 0);
 
   /* first handle the case where the user just wants to replace an old value */
   if (val && strchr (val, '='))
@@ -207,20 +314,18 @@ ostree_kernel_args_new_replace (OstreeKernelArgs *kargs,
       g_assert (new_val);
 
       guint i = 0;
-      if (!ot_ptr_array_find_with_equal_func (values, old_val, strcmp0_equal, &i))
+      if (!ot_ptr_array_find_with_equal_func (entries, old_val, kernel_args_entry_value_equal, &i))
         return glnx_throw (error, "No karg '%s=%s' found", key, old_val);
 
-      g_clear_pointer (&values->pdata[i], g_free);
-      values->pdata[i] = g_strdup (new_val);
+      kernel_args_entry_replace_value (entries->pdata[i], new_val);
       return TRUE;
     }
 
   /* can't know which val to replace without the old_val=new_val syntax */
-  if (values->len > 1)
+  if (entries->len > 1)
     return glnx_throw (error, "Multiple values for key '%s' found", key);
 
-  g_clear_pointer (&values->pdata[0], g_free);
-  values->pdata[0] = g_strdup (val);
+  kernel_args_entry_replace_value (entries->pdata[0], val);
   return TRUE;
 }
 
@@ -246,6 +351,13 @@ ostree_kernel_args_delete_key_entry (OstreeKernelArgs *kargs,
                                      const char       *key,
                                      GError          **error)
 {
+  GPtrArray *entries = g_hash_table_lookup (kargs->table, key);
+  if (!entries)
+    return glnx_throw (error, "No key '%s' found", key);
+  g_assert_cmpuint (entries->len, >, 0);
+
+  kernel_args_remove_entries_from_order (kargs->order, entries);
+
   if (!g_hash_table_remove (kargs->table, key))
     {
       g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
@@ -254,10 +366,6 @@ ostree_kernel_args_delete_key_entry (OstreeKernelArgs *kargs,
       return FALSE;
     }
 
-  /* Then remove the key from order table */
-  guint key_index;
-  g_assert (ot_ptr_array_find_with_equal_func (kargs->order, key, g_str_equal, &key_index));
-  g_assert (g_ptr_array_remove_index (kargs->order, key_index));
   return TRUE;
 }
 
@@ -294,16 +402,17 @@ ostree_kernel_args_delete (OstreeKernelArgs  *kargs,
   const char *key = arg_owned;
   const char *val = split_keyeq (arg_owned);
 
-  GPtrArray *values = g_hash_table_lookup (kargs->table, key);
-  if (!values)
+  GPtrArray *entries = g_hash_table_lookup (kargs->table, key);
+  if (!entries)
     return glnx_throw (error, "No key '%s' found", key);
-  g_assert_cmpuint (values->len, >, 0);
+  g_assert_cmpuint (entries->len, >, 0);
 
   /* special-case: we allow deleting by key only if there's only one val */
-  if (values->len == 1)
+  if (entries->len == 1)
     {
       /* but if a specific val was passed, check that it's the same */
-      if (val && !strcmp0_equal (val, values->pdata[0]))
+      OstreeKernelArgsEntry *e = entries->pdata[0];
+      if (val && !strcmp0_equal (val, _ostree_kernel_args_entry_get_value (e)))
         return glnx_throw (error, "No karg '%s=%s' found", key, val);
       return ostree_kernel_args_delete_key_entry (kargs, key, error);
     }
@@ -311,7 +420,7 @@ ostree_kernel_args_delete (OstreeKernelArgs  *kargs,
   /* note val might be NULL here, in which case we're looking for `key`, not `key=` or
    * `key=val` */
   guint i = 0;
-  if (!ot_ptr_array_find_with_equal_func (values, val, strcmp0_equal, &i))
+  if (!ot_ptr_array_find_with_equal_func (entries, val, kernel_args_entry_value_equal, &i))
     {
       if (!val)
         /* didn't find NULL -> only key= key=val1 key=val2 style things left, so the user
@@ -320,7 +429,8 @@ ostree_kernel_args_delete (OstreeKernelArgs  *kargs,
       return glnx_throw (error, "No karg '%s' found", arg);
     }
 
-  g_ptr_array_remove_index (values, i);
+  g_assert (g_ptr_array_remove (kargs->order, entries->pdata[i]));
+  g_assert (g_ptr_array_remove_index (entries, i));
   return TRUE;
 }
 
@@ -340,22 +450,38 @@ ostree_kernel_args_replace_take (OstreeKernelArgs   *kargs,
                                  char               *arg)
 {
   gboolean existed;
-  GPtrArray *values = g_ptr_array_new_with_free_func (g_free);
+  GPtrArray *entries = g_ptr_array_new_with_free_func (kernel_args_entry_free_from_table);
   const char *value = split_keyeq (arg);
   gpointer old_key;
 
-  g_ptr_array_add (values, g_strdup (value));
-  existed = g_hash_table_lookup_extended (kargs->table, arg, &old_key, NULL);
+  OstreeKernelArgsEntry *entry = g_new0 (OstreeKernelArgsEntry, 1);
+  _ostree_kernel_args_entry_set_value (entry, g_strdup (value));
+  g_ptr_array_add (entries, entry);
+
+  gpointer old_entries_ptr;
+  existed = g_hash_table_lookup_extended (kargs->table, arg, &old_key, &old_entries_ptr);
+  GPtrArray *old_entries = old_entries_ptr;
 
   if (existed)
     {
-      g_hash_table_replace (kargs->table, old_key, values);
-      g_free (arg);
+      g_assert (old_entries);
+      g_assert_cmpuint (old_entries->len, >, 0);
+
+      guint old_order_index = 0;
+      g_assert (ot_ptr_array_find_with_equal_func (kargs->order, old_key, kernel_args_entry_key_equal, &old_order_index));
+      kernel_args_remove_entries_from_order (kargs->order, old_entries);
+
+      g_assert_cmpstr (old_key, ==, arg);
+      _ostree_kernel_args_entry_set_key (entry, old_key);
+      g_ptr_array_insert (kargs->order, old_order_index, entry);
+      // `arg` is freed by the `g_hash_table_insert` call.
+      g_hash_table_insert (kargs->table, arg, entries);
     }
   else
     {
-      g_ptr_array_add (kargs->order, arg);
-      g_hash_table_replace (kargs->table, arg, values);
+      _ostree_kernel_args_entry_set_key (entry, arg);
+      g_hash_table_replace (kargs->table, arg, entries);
+      g_ptr_array_add (kargs->order, entry);
     }
 }
 
@@ -393,28 +519,26 @@ ostree_kernel_args_append (OstreeKernelArgs  *kargs,
                            const char        *arg)
 {
   gboolean existed = TRUE;
-  GPtrArray *values;
+  GPtrArray *entries = NULL;
   char *duped = g_strdup (arg);
   const char *val = split_keyeq (duped);
 
-  values = g_hash_table_lookup (kargs->table, duped);
-  if (!values)
+  entries = g_hash_table_lookup (kargs->table, duped);
+  if (!entries)
     {
-      values = g_ptr_array_new_with_free_func (g_free);
+      entries = g_ptr_array_new_with_free_func (kernel_args_entry_free_from_table);
       existed = FALSE;
     }
 
-  g_ptr_array_add (values, g_strdup (val));
+  OstreeKernelArgsEntry *entry = _ostree_kernel_args_entry_new ();
+  _ostree_kernel_args_entry_set_key (entry, duped);
+  _ostree_kernel_args_entry_set_value (entry, g_strdup (val));
+
+  g_ptr_array_add (entries, entry);
+  g_ptr_array_add (kargs->order, entry);
 
   if (!existed)
-    {
-      g_hash_table_replace (kargs->table, duped, values);
-      g_ptr_array_add (kargs->order, duped);
-    }
-  else
-    {
-      g_free (duped);
-    }
+    g_hash_table_replace (kargs->table, duped, entries);
 }
 
 /**
@@ -598,20 +722,13 @@ ostree_kernel_args_to_strv (OstreeKernelArgs *kargs)
 
   for (i = 0; i < kargs->order->len; i++)
     {
-      const char *key = kargs->order->pdata[i];
-      GPtrArray *values = g_hash_table_lookup (kargs->table, key);
-      guint j;
-
-      g_assert (values != NULL);
+      const char *key = _ostree_kernel_args_get_key_index (kargs, i);
+      const char *value = _ostree_kernel_args_get_value_index (kargs, i);
 
-      for (j = 0; j < values->len; j++)
-        {
-          const char *value = values->pdata[j];
-          if (value == NULL)
-            g_ptr_array_add (strv, g_strconcat (key, NULL));
-          else
-            g_ptr_array_add (strv, g_strconcat (key, "=", value, NULL));
-        }
+      if (value == NULL)
+        g_ptr_array_add (strv, g_strconcat (key, NULL));
+      else
+        g_ptr_array_add (strv, g_strconcat (key, "=", value, NULL));
     }
   g_ptr_array_add (strv, NULL);
 
@@ -644,27 +761,19 @@ ostree_kernel_args_to_string (OstreeKernelArgs *kargs)
 
   for (i = 0; i < kargs->order->len; i++)
     {
-      const char *key = kargs->order->pdata[i];
-      GPtrArray *values = g_hash_table_lookup (kargs->table, key);
-      guint j;
+      const char *key = _ostree_kernel_args_get_key_index (kargs, i);
+      const char *value = _ostree_kernel_args_get_value_index (kargs, i);
 
-      g_assert (values != NULL);
+      if (first)
+        first = FALSE;
+      else
+        g_string_append_c (buf, ' ');
 
-      for (j = 0; j < values->len; j++)
+      g_string_append (buf, key);
+      if (value != NULL)
         {
-          const char *value = values->pdata[j];
-
-          if (first)
-            first = FALSE;
-          else
-            g_string_append_c (buf, ' ');
-
-          g_string_append (buf, key);
-          if (value != NULL)
-            {
-              g_string_append_c (buf, '=');
-              g_string_append (buf, value);
-            }
+          g_string_append_c (buf, '=');
+          g_string_append (buf, value);
         }
     }
 
@@ -688,11 +797,12 @@ ostree_kernel_args_to_string (OstreeKernelArgs *kargs)
 const char *
 ostree_kernel_args_get_last_value (OstreeKernelArgs *kargs, const char *key)
 {
-  GPtrArray *values = g_hash_table_lookup (kargs->table, key);
+  const GPtrArray *entries = g_hash_table_lookup (kargs->table, key);
 
-  if (!values)
+  if (!entries)
     return NULL;
 
-  g_assert (values->len > 0);
-  return (char*)values->pdata[values->len-1];
+  g_assert (entries->len > 0);
+  const OstreeKernelArgsEntry *e = entries->pdata[entries->len-1];
+  return _ostree_kernel_args_entry_get_value (e);
 }

diff --git a/src/libostree/ostree-kernel-args.h b/src/libostree/ostree-kernel-args.h
index 3975ae5c10a88b3be0477f3da85695909cf4438c..5c8be0c02d52c840b6d9c598833a0d2eb2a0ac19 100644 (file)
--- a/src/libostree/ostree-kernel-args.h
+++ b/src/libostree/ostree-kernel-args.h
@@ -27,11 +27,40 @@
 G_BEGIN_DECLS
 
 typedef struct _OstreeKernelArgs OstreeKernelArgs;
+typedef struct _OstreeKernelArgsEntry OstreeKernelArgsEntry;
 
 GHashTable *_ostree_kernel_arg_get_kargs_table (OstreeKernelArgs *kargs);
 
 GPtrArray *_ostree_kernel_arg_get_key_array (OstreeKernelArgs *kargs);
 
+char *
+_ostree_kernel_args_entry_get_key (const OstreeKernelArgsEntry *e);
+
+char *
+_ostree_kernel_args_entry_get_value (const OstreeKernelArgsEntry *e);
+
+void
+_ostree_kernel_args_entry_set_key (OstreeKernelArgsEntry *e,
+                                   char  *key);
+
+void
+_ostree_kernel_args_entry_set_value (OstreeKernelArgsEntry *e,
+                                     char  *value);
+
+char *
+_ostree_kernel_args_get_key_index (const OstreeKernelArgs *kargs,
+                                   int i);
+
+char *
+_ostree_kernel_args_get_value_index (const OstreeKernelArgs *kargs,
+                                     int i);
+
+OstreeKernelArgsEntry *
+_ostree_kernel_args_entry_new (void);
+
+void
+_ostree_kernel_args_entry_value_free (OstreeKernelArgsEntry *e);
+
 _OSTREE_PUBLIC
 void ostree_kernel_args_free (OstreeKernelArgs *kargs);
 

diff --git a/tests/test-admin-deploy-karg.sh b/tests/test-admin-deploy-karg.sh
index aade011ce92b492ff72c95946c478cf51477d95b..ccf66b0e946343b7bc01cad2885450af79004402 100755 (executable)
--- a/tests/test-admin-deploy-karg.sh
+++ b/tests/test-admin-deploy-karg.sh
@@ -66,4 +66,8 @@ assert_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'option
 assert_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'options.*TESTARG=TESTVALUE'
 assert_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'options.*APPENDARG=VALAPPEND .*APPENDARG=2NDAPPEND'
 
+# Check correct ordering of different-valued args of the same key.
+${CMD_PREFIX} ostree admin deploy  --os=testos --karg-append=FOO=TESTORDERED --karg-append=APPENDARG=3RDAPPEND testos:testos/buildmaster/x86_64-runtime
+assert_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'options.*APPENDARG=VALAPPEND .*APPENDARG=2NDAPPEND .*FOO=TESTORDERED .*APPENDARG=3RDAPPEND'
+
 echo "ok deploy --karg-append"

diff --git a/tests/test-kargs.c b/tests/test-kargs.c
index 8d34f73c178779dfc026d76b751e56165abe3a79..d8370555fb43ca97eaf0e70af0a811fddb060c37 100644 (file)
--- a/tests/test-kargs.c
+++ b/tests/test-kargs.c
@@ -32,6 +32,22 @@ check_string_existance (OstreeKernelArgs *karg,
   return g_strv_contains ((const char* const*) string_list, string_to_find);
 }
 
+static gboolean
+kernel_args_entry_value_equal (gconstpointer data,
+                               gconstpointer value)
+{
+  const OstreeKernelArgsEntry *e = data;
+  return g_strcmp0 (_ostree_kernel_args_entry_get_value (e), value) == 0;
+}
+
+static gboolean
+kernel_args_entry_key_equal (gconstpointer data,
+                             gconstpointer key)
+{
+  const OstreeKernelArgsEntry *e = data;
+  return g_strcmp0 (_ostree_kernel_args_entry_get_key (e), key) == 0;
+}
+
 static void
 test_kargs_delete (void)
 {
@@ -82,7 +98,7 @@ test_kargs_delete (void)
   g_assert (ret);
   /* verify the value array is properly updated */
   GPtrArray *kargs_array = _ostree_kernel_arg_get_key_array (karg);
-  g_assert (!ot_ptr_array_find_with_equal_func (kargs_array, "single_key", g_str_equal, NULL));
+  g_assert (!ot_ptr_array_find_with_equal_func (kargs_array, "single_key", kernel_args_entry_value_equal, NULL));
   g_assert (!check_string_existance (karg, "single_key"));
 
   /* Delete specific key/value pair */
@@ -177,13 +193,6 @@ test_kargs_replace (void)
   g_assert (check_string_existance (karg, "test=newval"));
 }
 
-static gboolean
-strcmp0_equal (gconstpointer v1,
-               gconstpointer v2)
-{
-  return g_strcmp0 (v1, v2) == 0;
-}
-
 /* In this function, we want to verify that ostree_kernel_args_append
  * and ostree_kernel_args_to_string is correct. After that
  * we will use these two functions(append and tostring) in other tests: delete and replace
@@ -208,22 +217,22 @@ test_kargs_append (void)
     {
       if (g_str_equal (key, "test"))
         {
-          g_assert (ot_ptr_array_find_with_equal_func (value_array, "valid", strcmp0_equal, NULL));
-          g_assert (ot_ptr_array_find_with_equal_func (value_array, "secondvalid", strcmp0_equal, NULL));
-          g_assert (ot_ptr_array_find_with_equal_func (value_array, "", strcmp0_equal, NULL));
-          g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL, strcmp0_equal, NULL));
+          g_assert (ot_ptr_array_find_with_equal_func (value_array, "valid", kernel_args_entry_value_equal, NULL));
+          g_assert (ot_ptr_array_find_with_equal_func (value_array, "secondvalid", kernel_args_entry_value_equal, NULL));
+          g_assert (ot_ptr_array_find_with_equal_func (value_array, "", kernel_args_entry_value_equal, NULL));
+          g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL, kernel_args_entry_value_equal, NULL));
         }
       else
         {
           g_assert_cmpstr (key, ==, "second_test");
-          g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL, strcmp0_equal, NULL));
+          g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL, kernel_args_entry_value_equal, NULL));
         }
     }
 
   /* verify the value array is properly updated */
   GPtrArray *kargs_array = _ostree_kernel_arg_get_key_array (append_arg);
-  g_assert (ot_ptr_array_find_with_equal_func (kargs_array, "test", g_str_equal, NULL));
-  g_assert (ot_ptr_array_find_with_equal_func (kargs_array, "second_test", g_str_equal, NULL));
+  g_assert (ot_ptr_array_find_with_equal_func (kargs_array, "test", kernel_args_entry_key_equal, NULL));
+  g_assert (ot_ptr_array_find_with_equal_func (kargs_array, "second_test", kernel_args_entry_key_equal, NULL));
 
   /* Up till this point, we verified that the above was all correct, we then
    * check ostree_kernel_args_to_string has the right result